Feat/user crud unit tests#46
Conversation
maxzdosreis
left a comment
There was a problem hiding this comment.
Congratulations, I really liked the implementation. It just needs a few things aligned with the project standards, removing the comments, and some adjustments. It's also necessary to implement mappers (as mentioned in the comments) and implement the Swagger documentation for the controllers, always following the project standards. When submitting the PR for review, it's necessary to include evidence in the PR, such as approved tests, endpoint tests (e.g., the GET endpoint for /users in Postman), and a screenshot of the Swagger documentation.
There was a problem hiding this comment.
- Remove the entire ControllerExceptionHandler, as exception handling is already present in the GlobalExceptionHandler of the project. Therefore, you should add the handlers and exceptions to the GlobalExceptionHandler.
There was a problem hiding this comment.
ControllerExceptionHandler removed, Handlers moved to GlobalExceptionHandler.
| @ControllerAdvice | ||
| public class ControllerExceptionHandler { | ||
|
|
||
| @ExceptionHandler(MethodArgumentNotValidException.class) |
There was a problem hiding this comment.
- The handler for MethodArgumentNotValidException is duplicated; you should evaluate the other developer's implementation to see if it's feasible for your needs, and if necessary, make changes in a way that satisfies both your implementation and the other developer's.
There was a problem hiding this comment.
Refactored MethodArgumentNotValidException handler in GlobalExceptionHandler to replace this.
|
|
||
| return ResponseEntity.status(status).body(err); | ||
| } | ||
| @ExceptionHandler(DataIntegrityViolationException.class) |
There was a problem hiding this comment.
- Implement in GlobalExceptionHandler.
| return ResponseEntity.status(status).body(err); | ||
| } | ||
|
|
||
| @ExceptionHandler(DuplicateResourceValidationException.class) |
There was a problem hiding this comment.
- Implement in GlobalExceptionHandler.
There was a problem hiding this comment.
- Move the FieldMessage to api/src/main/java/com/orderflow/ecommerce/dtos/.
The record is an HTTP response template and should remain with the DTOS.
There was a problem hiding this comment.
FieldMessage moved to api/src/main/java/com/orderflow/ecommerce/dtos/
| } | ||
|
|
||
| @Test | ||
| void shouldUseSettersAndGetters() { |
There was a problem hiding this comment.
- Since the entity uses Lombok, I don't see the need to perform this test; it ends up being somewhat redundant, but it can be evaluated whether it should be removed or not.
| } | ||
|
|
||
| @Test | ||
| void shouldHaveNullsWhenUsingNoArgsConstructor() { |
There was a problem hiding this comment.
- It only checks id, name, and email for null values, but the no-args constructor leaves all fields as null. Either check all or check none—doing it halfway gives a false sense of coverage.
| } | ||
|
|
||
| @Test | ||
| void equalsAndHashCodeShouldBeBasedOnlyOnId() { |
There was a problem hiding this comment.
- Missing the scenario of two objects with null IDs, something like:
User d = new User();
User e = new User();
There was a problem hiding this comment.
- Did all the tests pass? Review the tests according to the necessary changes that will occur.
| @Test | ||
| void updateShouldThrowDuplicateResourceExceptionWhenEmailUsedByAnother() { | ||
|
|
||
| UserDto dto = new UserDto(existingId, "Bob New", "someoneelse@example.com", "pw", |
There was a problem hiding this comment.
- The password doesn't pass the regex, even though Bean Validation doesn't run in unit tests. Ideally, we should use a password that does pass the regex to simulate a real result.
|
I reviewed the PR and followed the discussion. Nice work on the implementation, Aline, and great review points from Max as well. Overall, I think this is moving in a good direction. Since this is Aline’s first PR in this project and the conversation has grown a bit, I think it would be helpful to summarize the remaining required changes before merging. @maxzdosreis, could you please add a short checklist in the main PR comment with the must-have items to close this PR? Ideally, only from the points already raised, without adding new items unless something is critical. From my side, one point I would also adjust is the Other than that, I believe the remaining items are already being addressed. Let’s focus on closing this task, and we can continue polishing/refactoring afterward if needed. Great work, everyone. Thanks for the commitment. |
Summary
Implemented the complete User CRUD feature, including validation, exception handling, and unit tests.
Changes
UserEntityUserControllerUserRepositoryUserDTOwith Bean ValidationControllerHandleException)Impact
This PR introduces the User module and establishes a reusable exception handling structure for future features.
Notes